#2146 — test(producer): gate source tests by execution lane

Verdict: LGTM 🟢

Introduces a test classification system for producer: every .test.ts under src/ is classified by runner (bun:test vs vitest, detected from imports) and lane (unit vs integration, from explicit manifest). Key SSOT properties:

• Single runner per file enforced. classifyTestSource rejects files importing both or neither runner — no ambiguity.
• Manifest staleness caught. discoverProducerTests validates INTEGRATION_TEST_FILES against what's on disk — a renamed/deleted test errors at classification time, not silently drops from the lane.
• Bun mock isolation. Each bun:test file runs in a separate process so mock.module registries don't leak. Vitest files batch into a single vitest run invocation (vitest handles isolation internally).

CI gets a new producer-source-tests job with matrix: [unit, integration] — clean separation from regression shards. The regression-harness.ts becomes import-safe via import.meta.url guard.

The captureStreamingStage.test.ts mock additions are engine API surface keeping — new exports like captureFrameToBufferPipelined, DrawElementVerificationError, getCapturePerfSummary etc. need stubs so the test compiles against the current engine. No behavioral change.

data-no-timeline on test fixture compositions is a correctness fix — minimal test compositions shouldn't trigger timeline binding.

Review by Miga

Final direct pass at current head b24c7bb. Verified the requested stack-specific behavior and no unresolved current review threads remain. Current CI lanes are green; any displayed regression failure is from a superseded run, while the latest shard set passes. Approved on code merits; retain stack order.

Reviewed at b24c7bbc.

Substantive test-infra work. The classifier + lane split is right — read through the whole surface. Highlights:

• Runner classification (test-classification.mjs:35) rejects both "imports neither" and "imports both" — no silent-drop default (feedback_dispatch_map_silent_drop_default_types safe). The INTEGRATION_TEST_FILES hardcoded allowlist is caught for staleness at test-classification.mjs:56 (throws if a listed file no longer exists), so drift is loud, not silent.
• Process-per-file for bun tests at run-test-lane.mjs:33 with the explanatory comment ("Bun's mock.module registry is process-global"). Correct hazard call-out; each .test.ts gets a clean process so mock.module from one file can't bleed into another.
• regression-harness.ts:1514 is-main-module guard — if (process.argv[1] && fileURLToPath(import.meta.url) === resolve(process.argv[1])). Prevents the harness from auto-running when imported by a unit test.
• Engine mock shape in captureStreamingStage.test.ts:23-33 — I verified against packages/engine/src/index.ts: captureFrameToBufferPipelined, captureFramesBatchPipelined, DrawElementVerificationError, completeDeferredDrawElementInit are all real exports (not feedback_external_dep_mock_shape_verify_runtime hallucinations). The locally-declared class DrawElementVerificationError extends Error {} is legitimately used as the mocked stand-in — the real class lives in frameCapture.ts:201 inside @hyperframes/engine.
• data-no-timeline additions to fixture divs (crossWorkerIdempotency.test.ts:44, renderChunk.test.ts:44) are anchored — the attribute is honored at engine/services/frameCapture.ts:1178, 1231. Legitimate opt-out for fixtures without GSAP timelines.
• allowSystemFontCapture: false in the 3 deterministicFonts-failClosed.test.ts cases: the option's declaration isn't in this PR (I couldn't find it on main), so it must have landed in an earlier stack PR. Assuming that's the case, explicit-false here is proper test-isolation to keep the unit lane from reaching into host font paths.

A couple of nits — none blocking:

• bun.lock version bumps 0.7.45 → 0.7.48 across ~11 packages (aws-lambda, cli, core, engine, gcp-cloud-run, lint, parsers, player, producer, sdk, shader-transitions, studio, studio-server) look like Graphite auto-sync noise, not test-infra intent. If they are — fine; worth a line in the PR body so reviewers don't hunt for a version-bump rationale.
• test:regression moved from producer/test → producer/test:regression. The top-level test now runs test:unit. Any tooling (docs, contributor CLAUDE.md, shell aliases, CI in adjacent repos) that ran bun run test expecting regression coverage now gets the unit lane. Not visible from this diff; heads-up worth landing with the batch.
• INTEGRATION_TEST_FILES allowlist grows manually. Staleness is caught but new-file adds require editing this list. Cheap alternative: describe.integration() marker + parser scan. Not for this PR — landing this as-is is fine and the manual list is loud enough that new tests won't accidentally slip to the unit lane.

LGTM from my side.

— Review by Rames D Jusso

Verdict: 🟢 LGTM at b24c7bb — with one scope-flag on the harness auto-run guard

Endorses Miga + Rames-D on classifier + lane split. The classifier at test-classification.mjs:35 fails loud on both "imports both runners" and "imports neither", and the INTEGRATION_TEST_FILES staleness check at test-classification.mjs:56 catches renames — those are good, no silent-drop paths. Adding one finding on top that neither reviewer surfaced: this PR carries a real bug fix that the test(producer): title doesn't advertise.

Findings

F1. Load-bearing scope hidden by title — regression-harness.ts auto-run guard. packages/producer/src/regression-harness.ts:1513-1523 moves the top-level void run().catch(...) behind if (process.argv[1] && fileURLToPath(import.meta.url) === resolve(process.argv[1])). This is not "gate tests by lane" — it's fixing a real side-effect bug where packages/producer/src/utils/streamDurationParity.test.ts:2 imports checkStreamDurationParity from ../regression-harness.js, and the prior top-level auto-run fired the entire regression suite as a module-init side effect during that test's import. The guard is load-bearing. Every prior legitimate caller (Dockerfile.test's bunx tsx src/regression-harness.ts … entrypoint, every producer test:* script) still triggers main() because process.argv[1] matches the harness path. Correct fix; the PR body should acknowledge this hunk exists.

F2. Classifier is direct-import only — theoretical, non-blocking. /\bfrom\s+["']bun:test["']/.test(source) at test-classification.mjs:35 doesn't chase transitive imports through helper modules. Today every producer .test.ts imports its runner directly (verified via search/code — all 20+ hits are the test files themselves). If a future author factors shared setup into _testHelpers.ts and moves the bun:test/vitest import there, the classifier will throw "imports neither runner" — fail-loud, so acceptable. Worth a comment near the regex noting the direct-import assumption.

F3. HYPERFRAMES_TEST_LANE — set but not read (info). Phase-1 recon anticipated per-file env gates. Actual behavior: run-test-lane.mjs:22 sets HYPERFRAMES_TEST_LANE in the sub-process env; no .test.ts in the diff reads it as a runtime skip gate. The 4 modified test files got unrelated mock-signature fixes (deterministicFonts allowSystemFontCapture, data-no-timeline, expanded mock.module(...) shapes) to make them lane-runnable. No inconsistency issue.

F4. CI changes path filter covers producer-source-tests inputs — verified. ci.yml paths-filter code includes packages/**, scripts/**, package.json, bun.lock, tsconfig*.json, Dockerfile*, .github/workflows/** — so packages/producer/scripts/** and packages/producer/**/*.test.ts route to the guarded producer-source-tests job (ci.yml:238) correctly.

Adversarial pass

F1 initially read as a scope-drift request-changes finding; downgraded to LGTM-with-scope-flag because the code IS right and the title-fix ask is a body edit, not a new commit. F2 is theoretical (no such helper exists today) — kept only as a comment-worthy nit. Considered flagging the auto-run guard change as a Windows-path edge case (resolve() case-sensitivity vs fileURLToPath on C:\...) — dropped because HF's CI runs on Linux and Windows dev isn't a supported target.

R1 by Via

Post-merge advisory review (this PR is already merged into main). Findings flagged for follow-up, not gating.

Reviewed the final head against its actual parent 8c21736a, including the CI job and local composite action it invokes. No new findings.

• packages/producer/scripts/test-classification.mjs:34-63 assigns every discovered source test exactly one runner, rejects both/neither imports, and fails on stale integration-manifest entries.
• packages/producer/scripts/run-test-lane.mjs:27-34 batches Vitest while isolating each Bun test in a fresh process, preserving mock.module isolation.
• The new CI lane’s prepare-ffmpeg-bin composite contains no external download indirection; it only copies the runner binary/stub before install.
• Stack boundary is clean: this final head is one direct commit over a base that already contains #2145.

Verification on the cumulative compatible head: classification passed (53 files after #2147’s one additional integration test), the full producer unit lane passed (18 Vitest files / 283 tests plus all 23 isolated Bun files), and 4 Vitest integration files / 12 tests passed. The local Bun integration lane hit host FFmpeg-4 limitations (fps_mode unsupported / rate mismatch), while all 56 exact final-head GitHub checks—including the provisioned integration lane—are green; this is environment evidence, not a PR regression.

Verdict: COMMENT (post-merge: Ready)
Reasoning: Test discovery is fail-loud, runner isolation is correct, the stack boundary is clean, and exact-head CI provides the environment-correct integration proof.

— Deepwork